Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Align preallocationSize behavior #58726

Merged
merged 9 commits into from
Sep 14, 2021
Merged

Align preallocationSize behavior #58726

merged 9 commits into from
Sep 14, 2021

Conversation

adamsitnik
Copy link
Member

@adamsitnik adamsitnik commented Sep 6, 2021 •
edited
Loading

Align Windows preallocationSize implementation to act like Unix implementation (which is less flexible and we can't make it work in other direction):

  • modify EOF no matter if anything is later written to file or not (which is reported by FileStream.Length)
  • zero the file contents

By using these benchmarks I've confirmed that there is no perf regression (to my suprise, I've double checked that)

I've also fixed an OSX bug, where we could have accidentally shrink a file opened with FileMode.OpenOrCreate

@adamsitnik
there should be no differences between OSes:
7d64626 
* file length should always be set
* file content should always be zeroed
* OpenOrCreate can't shrink an existing file
@adamsitnik
align Windows implementation to behave like the Unix one
44f1786
@adamsitnik adamsitnik requested a review from jozkee September 8, 2021 07:05
jozkee
jozkee approved these changes Sep 9, 2021
View reviewed changes
Copy link
Member

@jozkee jozkee left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Otherwise, LGTM. Thanks.

@stephentoub
Copy link
Member

Align Windows preallocationSize implementation to act like Unix implementation

This will be a behavioral change on Windows. We should either get this into .NET 6 or reconsider whether this is a break we want to take in .NET 7.

@adamsitnik
Copy link
Member Author

This will be a behavioral change on Windows.

It will, but for the good reasons (same behavior on every platform).

We should either get this into .NET 6 or reconsider whether this is a break we want to take in .NET 7.

I think that we should include it in .NET 6, as currently nobody is using this API (because it's very new) and we avoid confusion (different behavior on different platforms) and introducing breaking changes in .NET 7.

@stephentoub
Copy link
Member

same behavior on every platform

I agree. My point is... I suggest you get buy-off about getting this into .NET 6 before merging it into .NET 7.

@adamsitnik adamsitnik force-pushed the alignPreallocationSize branch from ba7f512 to 81fbd92 Compare September 13, 2021 10:32
stephentoub
stephentoub reviewed Sep 13, 2021
View reviewed changes
src/libraries/System.Private.CoreLib/src/System/IO/Strategies/FileStreamHelpers.cs Outdated
=> preallocationSize > 0
&& (access & FileAccess.Write) != 0
&& mode != FileMode.Open && mode != FileMode.Append;
&& mode != FileMode.Open && mode != FileMode.Append
&& (mode != FileMode.OpenOrCreate || fileHandle.CanSeek && RandomAccess.GetFileLength(fileHandle) == 0); // allow to extend only new files
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: please add parens around the latter expression... precedence between || and && is hard to remember.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just because a file length is 0 doesn't mean it's new.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just because a file length is 0 doesn't mean it's new.

I agree, but I am not sure if there is a reliable way of telling that given file is new.

@runfoapp runfoapp bot mentioned this pull request Sep 13, 2021
Closed
@adamsitnik adamsitnik merged commit 6115ca2 into dotnet:main Sep 14, 2021
@adamsitnik adamsitnik deleted the alignPreallocationSize branch September 14, 2021 11:30
@adamsitnik
Copy link
Member Author

/backport to release/6.0

@github-actions
Copy link
Contributor

Started backporting to release/6.0: https://github.com/dotnet/runtime/actions/runs/1233431311

@github-actions github-actions bot mentioned this pull request Sep 14, 2021
Closed
@stephentoub stephentoub mentioned this pull request Sep 20, 2021
Merged
@jozkee jozkee mentioned this pull request Sep 23, 2021
Merged
jozkee pushed a commit to jozkee/runtime that referenced this pull request Sep 24, 2021
@adamsitnik @stephentoub @jozkee
Align preallocationSize behavior (dotnet#58726)
ee72add 
Co-authored-by: Stephen Toub <[email protected]>
Anipik pushed a commit that referenced this pull request Sep 24, 2021
@jozkee @adamsitnik
@stephentoub @tmds
[release/6.0-rc2] File preallocationSize: align Windows and Unix beha…
157d591 
…vior. (#59532)

* Align preallocationSize behavior (#58726)

Co-authored-by: Stephen Toub <[email protected]>

* File preallocationSize: align Windows and Unix behavior. (#59338)

* File preallocationSize: align Windows and Unix behavior.

This aligns Windows and Unix behavior of preallocationSize for the
intended use-case of specifing the size of a file that will be written.

For this use-case, the expected FileAccess is Write, and the file should be
a new one (FileMode.Create*) or a truncated file (FileMode.Truncate).
Specifing a preallocationSize for other modes, or non-writable files throws ArgumentException.

The opened file will have a length of zero, and is ready to be written to by the user.

If the requested size cannot be allocated, an IOException is thrown.

When the OS/filesystem does not support pre-allocating, preallocationSize is ignored.

* fix pal_io preprocessor checks

* pal_io more fixes

* ctor_options_as.Windows.cs: fix compilation

* Update tests

* tests: use preallocationSize from all public APIs

* pal_io: add back FreeBSD, fix OSX

* tests: check allocated is zero when preallocation is not supported.

* Only throw for not enough space errors

* Fix compilation

* Add some more tests

* Fix ExtendedPathsAreSupported test

* Apply suggestions from code review

Co-authored-by: David Cantú <[email protected]>

* Update System.Private.CoreLib Strings.resx

* PR feedback

* Remove GetPathToNonExistingFile

* Fix compilation

* Skip checking allocated size on mobile platforms.

Co-authored-by: David Cantú <[email protected]>

* Fix unused fileDescriptor

* void fd when unused

* Address feedback

Co-authored-by: Adam Sitnik <[email protected]>
Co-authored-by: Stephen Toub <[email protected]>
Co-authored-by: Tom Deseyn <[email protected]>
@adamsitnik adamsitnik mentioned this pull request Oct 11, 2021
Closed
@stephentoub stephentoub mentioned this pull request Oct 12, 2021
Merged
@ghost ghost locked as resolved and limited conversation to collaborators Nov 3, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@jozkee jozkee jozkee approved these changes

@stephentoub stephentoub Awaiting requested review from stephentoub

Assignees
No one assigned
Labels
area-System.IO
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@adamsitnik @stephentoub @jozkee